Rustify `ci/integration.sh` script by GuillaumeGomez · Pull Request #6817 · rust-lang/rustfmt

GuillaumeGomez · 2026-03-04T18:03:02Z

As discussed on zulip.

I went the easy road: didn't create a workspace for this script, so it uses the same dependencies as the main project (although it only needs std). It can be run with cargo run --bin ci-integration.

Anyway, this is very straightforward conversion, please tell me if you'd prefer things to be different in any regard.

r? @jieyouxu

matthewhughes934

All my style comments are subjective, so feel free to ignore at your discretion

View changes since this review

ci/integration.rs

matthewhughes934 · 2026-03-05T21:26:00Z

ci/integration.rs

+    // FIXME: this means we can get a stale cargo-fmt from a previous run.
+    //
+    // `which rustfmt` fails if rustfmt is not found. Since we don't install
+    // `rustfmt` via `rustup`, this is the case unless we manually install it. Once
+    // that happens, `cargo install --force` will be called, which installs
+    // `rustfmt`, `cargo-fmt`, etc to `~/.cargo/bin`. This directory is cached by
+    // travis (see `.travis.yml`'s "cache" key), such that build-bots that arrive
+    // here after the first installation will find `rustfmt` and won't need to build
+    // it again.
+    //
+    //which cargo-fmt || cargo install --force


Is this comment outdated now: do things still run in Travis? Does Github actions have the same caching that might cause an issue here?

Absolutely no idea. I kept the existing comments as is.

We should remove this, as far as I know rust-lang/* CI has not been using Travis for a good while.

Would be good if one of the maintainers can double-check.

In the GHA case this is only true if we explicitly opt-in to actions cache on ~/.cargo/bin, i.e.

https://github.com/actions/cache/blob/main/examples.md#rust---cargo

Looking at https://github.com/rust-lang/rustfmt/blob/main/.github/workflows/integration.yml we don't use any caching so this should be fine.

matthewhughes934 · 2026-03-05T21:32:27Z

ci/integration.rs

+    }
+
+    let output = run_command_with_output_and_env("cargo", &["test", test_args], current_dir, &env)?;
+    if ["build failed", "test result: FAILED."]


Will we get here? If tests or builds fail I assume cargo test will exit non-zero and in that case will run_command_with_output_and_env above return an Err?

I kept the same behaviour as before: https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L46-L48

If the output contains one of the two strings, we return the function with 0, meaning success. Sounds super weird but that's the current behaviour.

Btw, run_command_with_output_and_env doesn't check whether or not the command exited successfully.

My guess is that the intention there is to skip over crates which already fails test (if the baseline fails it could be other reasons too).

ci/integration.rs

matthewhughes934 · 2026-03-05T21:37:48Z

ci/integration.rs

+    )?;
+    write_file("rustfmt_check_output", &output)?;
+
+    run_command_with_env("cargo", &["test", test_args], current_dir, &env)


Don't we already run cargo test at line 118?

Yep. But kept the exact same behaviour:

https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L45

https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L73

This might catch really broken --check format where it should not have modified the source in some off chance, seems okay to keep it. Can we add a comment here to say sth to that effect? I imagine we won't be the only ones with that question reading this :)

ci/integration.rs

GuillaumeGomez · 2026-03-06T10:31:03Z

Applied suggestions and fixed file creation.

matthewhughes934

I think I didn't read the original script carefully enough on my first review (I think most my confusion comes from things that script was already doing, sorry about that). So this review is more focused on changes in behaviour from the old script

(also Github isn't letting me resolve some of the conversations your answered? I don't see the button for it for some reason)

View changes since this review

ci/integration.rs

GuillaumeGomez · 2026-03-09T14:20:40Z

Updated!

ci/integration.rs

GuillaumeGomez · 2026-03-10T10:57:34Z

Applied suggestion.

jieyouxu

Sorry for the delay. This broadly looks good to me, some nits.
@rustbot author

View changes since this review

jieyouxu · 2026-03-05T01:19:49Z

.github/workflows/integration.yml

        INTEGRATION: ${{ matrix.integration }}
        TARGET: x86_64-unknown-linux-gnu
-      run: ./ci/integration.sh
+      run: cargo run --bin ci-integration


Remark (not for this PR): I wonder if we should coalsce this and Diff Check into one crate, sth akin to citool on r-l/r. The benefit is that the tools could then share common logic easily.

That's what I plan to do. But to reduce the diff, I decided to do it in multiple parts:

Migrate shell scripts to rust

Improve code

Although I think I'll make one crate once I start porting the second one to not duplicate code.

jieyouxu · 2026-03-10T11:52:58Z

ci/integration.rs

+    bin: &str,
+    args: I,
+    current_dir: &str,
+    env: &HashMap<&str, &str>,


Remark: thought about BTreeMap for stable iteration order but I think that might not matter

jieyouxu · 2026-03-10T11:58:50Z

ci/integration.rs

+    }
+
+    let output = run_command_with_output_and_env("cargo", &["test", test_args], current_dir, &env)?;
+    if ["build failed", "test result: FAILED."]


My guess is that the intention there is to skip over crates which already fails test (if the baseline fails it could be other reasons too).

jieyouxu · 2026-03-10T12:00:39Z

ci/integration.rs

+
+    let output =
+        run_command_with_output_and_env("cargo", &["fmt", "--all", "-v"], current_dir, &env)?;
+    println!("{}", output.output);


Thought (not for this PR): we might be able to help group the logs with GHA log groups

jieyouxu · 2026-03-10T12:02:20Z

ci/integration.rs

+    )?;
+    write_file("rustfmt_check_output", &output)?;
+
+    run_command_with_env("cargo", &["test", test_args], current_dir, &env)


This might catch really broken --check format where it should not have modified the source in some off chance, seems okay to keep it. Can we add a comment here to say sth to that effect? I imagine we won't be the only ones with that question reading this :)

ci/integration.rs

jieyouxu · 2026-03-10T12:08:32Z

ci/integration.rs

+    // FIXME: this means we can get a stale cargo-fmt from a previous run.
+    //
+    // `which rustfmt` fails if rustfmt is not found. Since we don't install
+    // `rustfmt` via `rustup`, this is the case unless we manually install it. Once
+    // that happens, `cargo install --force` will be called, which installs
+    // `rustfmt`, `cargo-fmt`, etc to `~/.cargo/bin`. This directory is cached by
+    // travis (see `.travis.yml`'s "cache" key), such that build-bots that arrive
+    // here after the first installation will find `rustfmt` and won't need to build
+    // it again.
+    //
+    //which cargo-fmt || cargo install --force


We should remove this, as far as I know rust-lang/* CI has not been using Travis for a good while.

Would be good if one of the maintainers can double-check.

In the GHA case this is only true if we explicitly opt-in to actions cache on ~/.cargo/bin, i.e.

https://github.com/actions/cache/blob/main/examples.md#rust---cargo

Looking at https://github.com/rust-lang/rustfmt/blob/main/.github/workflows/integration.yml we don't use any caching so this should be fine.

GuillaumeGomez · 2026-03-10T13:08:09Z

Applied suggestions, I also added comments to explain the final command as well.

jieyouxu

Looks good to me with one tiny nit, thanks

View changes since this review

jieyouxu · 2026-03-11T01:35:21Z

Cargo.toml

+[[bin]]
+name = "ci-integration"
+path = "ci/integration.rs"


Nit: can we move this up to be alongside the other [[bin]]s?

Sure, done!

rustbot assigned jieyouxu Mar 4, 2026

rustbot added A-CI Area: CI S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Mar 4, 2026

matthewhughes934 reviewed Mar 5, 2026

View reviewed changes

GuillaumeGomez force-pushed the rustify-shell branch from 980b07b to 0469d7c Compare March 6, 2026 10:30

matthewhughes934 reviewed Mar 6, 2026

View reviewed changes

ci/integration.rs Outdated Show resolved Hide resolved

ci/integration.rs Show resolved Hide resolved

ci/integration.rs Show resolved Hide resolved

matthewhughes934 approved these changes Mar 10, 2026

View reviewed changes

ci/integration.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the rustify-shell branch from 4d05d5e to eeb5fe5 Compare March 10, 2026 10:57

jieyouxu reviewed Mar 10, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Mar 10, 2026

jieyouxu approved these changes Mar 11, 2026

View reviewed changes

GuillaumeGomez added 5 commits March 11, 2026 11:51

Rustify ci/integration.sh script

e1f0c8e

Replace map! macro with plain HashMap calls

04840f6

Only create rustfmt.toml if it does not already exist

5706ca5

Stick closer to original shell script behaviour

f07689e

Improve ci/integration.rs code

1789617

GuillaumeGomez force-pushed the rustify-shell branch from 32e6ec5 to 1789617 Compare March 11, 2026 10:52

Conversation

GuillaumeGomez commented Mar 4, 2026

Uh oh!

matthewhughes934 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 6, 2026

Uh oh!

matthewhughes934 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 9, 2026

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 10, 2026

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Mar 10, 2026

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 left a comment •

edited by rustbot

Loading

jieyouxu Mar 10, 2026 •

edited

Loading

matthewhughes934 left a comment •

edited by rustbot

Loading

jieyouxu left a comment •

edited by rustbot

Loading

jieyouxu Mar 10, 2026 •

edited

Loading

jieyouxu left a comment •

edited by rustbot

Loading